V12 ZK-E2E - #648
Merged
Merged
Conversation
The reviewed finding (exit credits recorded twice via the event-scanning WormholeProofRecorderExtension) is not reproducible: exits are bare extrinsics, so the extension's post_dispatch never runs for them, and Unbalanced::increase_balance emits no Minted/Transfer event to scan. Add regression tests pinning both guards and document why the exit credit must stay on the event-free increase_balance path. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the independent endowed_addresses genesis vector and GenesisEndowmentsPending staging. At block 1, record transfer proofs from every account that exists with a balance, so exit capacity cannot disagree with actually issued genesis value. Co-authored-by: Cursor <cursoragent@cursor.com>
…ches The aggregator rebate is deliberately permissionless (whoever aggregates names its own payout address), which is safe because the address is a proof public input: redirecting it invalidates the proof at the pre_dispatch block-inclusion gate. Add a regression test pinning that property. Co-authored-by: Cursor <cursoragent@cursor.com>
The on-chain zk-tree can grow to depth 32 but the wormhole circuits accept Merkle paths only up to depth 16 (~4.3B leaves). Document that this is a deliberate proving-cost trade-off, the timeline to exhaustion (years to centuries depending on transfer volume), and the circuit update + runtime upgrade planned when the limit approaches. Also fixes the MAX_TREE_DEPTH comment that wrongly claimed circuits support depth 32. Co-authored-by: Cursor <cursoragent@cursor.com>
…ights The wormhole settlement weights, the transfer-proof extension's per_transfer_weight, and reversible-transfers' execute_transfer charged the leaf insert's DB ops but not its depth-dependent Poseidon hashing (one hash per tree level), under-declaring execution work per recorded transfer. Add a live-depth insert_leaf_hash_ref_time helper to pallet-zk-tree (mirroring insert_leaf_db_ops) and charge it on all three routes, pinned by depth-sensitivity tests. Co-authored-by: Cursor <cursoragent@cursor.com>
A zero-value transfer_keep_alive (or zero-value scheduled transfer) emitted a Transfer event that the proof recorder turned into a useless ZK-tree leaf, advancing transfer counts and growing the tree with no value movement. Guard the recorder chokepoint (report the credit as deliberately dropped) and reject zero-amount schedules at the reversible-transfers entry points. Co-authored-by: Cursor <cursoragent@cursor.com>
…ign) Genesis JSON is supplied by the node operator building their own chain spec; it is not an untrusted boundary, so size limits there would protect no one. Note this on prepare_genesis_build_input where the next reviewer will look. Co-authored-by: Cursor <cursoragent@cursor.com>
BuildGenesisConfig::build returns () — panicking assertions are the only failure channel for invalid genesis data, inherited verbatim from upstream Substrate, and they abort the operator's own chain-spec build. Extend the trust-model note so the next reviewer finds this answered. Co-authored-by: Cursor <cursoragent@cursor.com>
… wormhole The proof-recorder extension only matched Balances::Transfer and Balances::Minted, but reversible-transfers releases seized/recovered held funds with transfer_on_hold, which emits Balances::TransferOnHold. The guardian therefore received spendable free balance with no ZK-tree leaf. Match TransferOnHold in the event scan; weight is already reconciled post-dispatch for statically uncountable paths. Co-authored-by: Cursor <cursoragent@cursor.com>
…re) in wormhole Same class as the TransferOnHold fix: pallet_recovery::close_recovery moves the rescuer's reserved deposit to the rescued account with repatriate_reserved, which emits Balances::ReserveRepatriated — an event the proof-recorder extension ignored, so the credit got no ZK-tree leaf. Match ReserveRepatriated in the event scan. Sweep of other balance-crediting APIs found no further gaps (referenda slashes burn to (), fee flows are excluded by design, TransferAndHold is unused). Co-authored-by: Cursor <cursoragent@cursor.com>
The extension doc claimed scheduler-dispatched transfers were automatically covered, but extensions never run for hook-context dispatch. Spell out the actual contract: signed-extrinsic events are scanned; hook-context credits record explicitly (reversible-transfers execution, mining rewards/treasury); and governance enactment via the scheduler is a known, accepted gap because only Root can reach it and Root can already forge leaves via set_storage. Co-authored-by: Cursor <cursoragent@cursor.com>
resolve_proof_block accepted any backend-resolvable hash, so side-fork blocks (and fork blocks above best, where the one-sided window check saturates to 0) produced proof material that settlement always rejects against frame_system::BlockHash. Reject heights above best and require the requested hash to match the canonical hash at its height. The test mock now distinguishes imported blocks from the canonical index so fork-block cases are expressible. Co-authored-by: Cursor <cursoragent@cursor.com>
pre_validate_{private,public}_batch_proof copied and parsed unbounded
attacker bytes on the fee-free unsigned path, and plonky2's from_bytes
silently ignores trailing bytes, giving one proof unboundedly many byte
representations that each re-cost copy+parse at pool admission. Gate
length at MAX_PROOF_BYTES (512 KiB vs ~151/224 KB real proofs) before
any copy, and require the bytes to round-trip through to_bytes so each
proof has exactly one accepted encoding. Release-mode recalibration
shows the existing pre-validation weight constants still cover the
added serialize pass with large margin.
Co-authored-by: Cursor <cursoragent@cursor.com>
…h weights is_call_allowed hides an is_high_security classification read; as_derivative and as_recovered enforced the policy without charging the read, and Multisig::propose paid it twice (weight selection + is_call_allowed) while its weights document one read. Split the policy predicate into is_call_allowed_given so propose reuses its fetched classification, and add the read to the two wrapper weight declarations (and as_derivative's actual-weight mirror). Recovery's mock gets a non-zero DbWeight so the new weight assertions are meaningful. Co-authored-by: Cursor <cursoragent@cursor.com>
record_proofs_from_events_since stream-decodes every event record present at scan time (Iterator::skip discards but still decodes the pre-snapshot prefix), yet weight() only charges per counted transfer and the post-hoc registration only fired for recording shortfalls — batched remark_with_event traffic produced unmetered linear decode work. Register event_scan_weight (one Events read + a conservative 1µs/record decode ceiling) alongside the existing shortfall in post_dispatch. Co-authored-by: Cursor <cursoragent@cursor.com>
…st budget) The QPoW header commits a fixed 110-byte digest window that the pre-runtime item and seal fill exactly, and import rejects anything larger. Upstream frame-system's RuntimeEnvironmentUpdated deposit on set_code/set_heap_pages therefore made every environment-changing block unimportable network-wide — runtime upgrades could not be finalized through normal block production. Nothing in the node stack consumes the item (clients detect upgrades from the :code state key, not the digest), so remove both deposits in the fork, document the no-runtime-digest-items invariant on deposit_log and DIGEST_LOGS_SIZE, and flip the pallet tests to assert the digest stays empty. Co-authored-by: Cursor <cursoragent@cursor.com>
n13
approved these changes
Aug 7, 2026
n13
left a comment
Collaborator
There was a problem hiding this comment.
Review — security fixes: wormhole, settlement, metering
Reviewed the full diff on the checked-out branch (illuzen/v12-zk-e2e), verified the load-bearing claims against the code, and ran the affected suites locally. Together with qp-zk-circuits#169 (reviewed there as well).
Verified independently
- QPoW digest window: block import rejects oversized digests at
client/consensus/qpow/src/lib.rs:238and:407, and nothing in the node stack consumesRuntimeEnvironmentUpdated. Theframe-executivedeposit_log(frame/executive/src/lib.rs:1018) re-deposits the block's own pre-runtime digest, so removing the runtime deposits is safe — and the fix removes a genuine "runtime upgrades can't land" liveness bug. - Event semantics:
TransferOnHold/ReserveRepatriatedfield names and credit direction match the vendored balances pallet (pallets/balances/src/lib.rs:418,:373) and the seizure (pallets/reversible-transfers/src/lib.rs:936) /close_recoverycall sites.TransferAndHoldis indeed never emitted outside balances' own tests. - Zero-amount rejection covers both scheduling entry points via the shared
do_schedule_transfer_inner(pallets/reversible-transfers/src/lib.rs:762). - Policy-read weights:
as_recovered(pallets/recovery/src/lib.rs:456) andas_derivativedo callis_call_allowed→ one classification read each, matching the addedDbWeight::reads(1).Multisig::proposereuses the classification already fetched for the same account — sound. - Event-scan metering:
events_at_scanis captured before recording deposits new events, and equals the number of records the scan actually decodes (skipdecodes the discarded prefix). Zero-amount credits returningfalsekeeps weight reconciliation consistent. - Proof framing:
MAX_PROOF_BYTES(512 KiB) vs. real fixture sizes (~151/224 KB) leaves sane headroom, and the round-trip check pins exactly one accepted encoding per proof.
Test runs (this machine)
pallet-zk-tree,pallet-reversible-transfers,pallet-utility,pallet-recovery,pallet-multisig,frame-system: 281 passed, 0 failed.pallet-wormhole --release --lib: 68 passed, 0 failed (3 ignored fixture-regen tests).quantus-runtime --release --lib: 33 passed, 0 failed.
Non-blocking observations
on_initialize(1)(pallets/wormhole/src/lib.rs): the declared weight counts account iteration + 2r/2w per recorded proof but not the per-leaf ZK-tree insert (DB ops + Poseidon hashing). Same under-counting shape as the old code, one-time hook at block 1, harmless in practice — noting for the next weight pass.- Genesis derivation changes the recorded set from "explicit endowed list" to "every account with a balance at block 1". For the shipped presets the sets are identical (the old config was fed the same
balanceslist); any pallet account pre-funded outsidebalanceswould now gain a leaf keyed to an address nobody holds a secret for — dead weight, not a risk, since ZK-spending requires the account secret. node/src/zktree_rpc.rs: error code 9006 now covers two distinct backend-failure modes (number resolution and canonical-hash resolution). Both are backend failures so it's arguably consistent, just slightly less precise for API consumers.- The per-block quadratic scan cost (each
post_dispatchre-decodes every event record present) is now metered per transaction, so block-weight accounting is sound — worth keeping an eye on as event volumes grow.
Verdict
APPROVE — root-cause fixes, each pinned by a regression test, claims hold up under independent checking. No blocking issues.
Collaborator
|
Ok Codex 5.6 also approved this (post merge) - just ran a check |
n13
added a commit
that referenced
this pull request
Aug 7, 2026
Resolve genesis_config_presets conflict: keep vesting genesis schedules and pot endowment; drop wormhole endowed_addresses (proofs now derive from balances at block 1 per #648).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security review: wormhole, settlement, and metering fixes
This PR addresses a batch of security-review findings against the V12 wormhole / ZK-tree
stack. Each item was first verified against the code, reproduced with a red test where
applicable, and fixed at the root cause; findings that turned out to be intended design or
operator-trusted boundaries were documented where the next reviewer will look instead.
Findings touching the turnstile (
PotentialWormholeBalancereveal/ambiguity machinery) wereskipped here — the turnstile is removed in a separate PR.
Fixes
Proof recording completeness
903c82e3) — Verified the reporteddouble-record (extension event scan +
process_exit_bundle) is not reachable: bareextrinsics never run
post_dispatch, andincrease_balanceemits noMintedevent.Removed the dead
bare_post_dispatchoverride that suggested otherwise and addedregression tests pinning both facts.
0e7f4b6f) — Reversible-transfer guardian seizures andrecoveries move value with
transfer_on_hold, which emitsBalances::TransferOnHoldrather than
Transfer; the recorder ignored it, so the guardian's credit got no ZK-treeleaf. The event matcher now records it like any transfer.
1cbc1b4b) — Same class:recovery::close_recoveryseizes the rescuer's deposit viarepatriate_reserved(
Balances::ReserveRepatriated), which the recorder also ignored. A sweep of all otherbalance-crediting APIs found no further gaps (referenda slashes burn to
(), fee flowsare excluded by design,
TransferAndHoldis unused).14d7fbe0) — Zero-value transfers (reachable fromplain
transfer_keep_alive(0)and scheduled transfers) created pure-state-growth leaves.record_transfer_proofnow drops zero-amount credits at the single chokepoint, andpallet-reversible-transfersrejects zero-amount schedules outright.Genesis
86612f34) — The wormhole pallet'sindependent
endowed_addressesgenesis vector could disagree withpallet_balances(duplicate, oversized, or missing entries), creating exit capacity for value never
issued. The vector is gone; the pallet now derives its block-1 transfer proofs directly
from the actual balances genesis, making mismatch unrepresentable.
Settlement admission
ca9da896) — The rebate to thepublic-batch aggregator is intended design (a portion of the volume fee), but the
aggregator address binding is now pinned by a pre-dispatch regression test against
redirection.
bc66b510) —proof_bytesreached
to_vec()+ plonky2from_bytesunbounded on the fee-free unsigned path, andthe parser silently ignores trailing bytes, giving one proof unboundedly many byte
representations that each re-cost a full copy + parse at pool admission. Pre-validation
now gates length at
MAX_PROOF_BYTES(512 KiB vs ~151/224 KB real proofs) before anycopy, and requires the bytes to round-trip through
to_bytes()so each proof has exactlyone accepted encoding. Release-mode recalibration confirmed the existing pre-validation
weight constants still cover the added serialize pass.
RPC
cd045424) —resolve_proof_blockaccepted any backend-resolvable hash, so side-fork blocks (and fork blocks above best,
where the one-sided window check saturates to zero) produced proof material that
settlement always rejects against
frame_system::BlockHash. The resolver now rejectsheights above best and requires the requested hash to equal the canonical hash at its
height.
Weight / metering
80802efa) — Wormholesettlement, the proof-recorder extension, and reversible-transfer execution all charged
the leaf insert's DB ops but not its depth-proportional Poseidon hashing. A new
insert_leaf_hash_ref_time()helper onpallet-zk-treeis now charged on all threeroutes, tracking the live tree depth.
50c5f0b1) —is_call_allowedhides anis_high_securitystorage read;as_derivativeandas_recoveredenforced the policywithout charging it, and
Multisig::proposepaid it twice while documenting one read.The policy predicate is split into
is_call_allowed_givensoproposereuses itsfetched classification, and both wrapper weights now charge the read.
c35ec302) — The recorder's event scanstream-decodes every event record present at scan time (
Iterator::skipstill decodesthe prefix it discards), which was unmetered — batched
remark_with_eventtrafficproduced free decode work. The scan is now registered against the block
(
event_scan_weight: oneEventsread + a conservative 1µs/record ceiling) alongsidethe existing recording-shortfall mechanism.
Consensus
RuntimeEnvironmentUpdatedno longer deposited (9dffec26) — The QPoW headercommits a fixed 110-byte digest window that the pre-runtime item and seal fill exactly,
and import rejects anything larger. Upstream frame-system's 1-byte
RuntimeEnvironmentUpdateddeposit onset_code/set_heap_pagestherefore made everyenvironment-changing block unimportable network-wide — runtime upgrades could not land
through normal block production. Nothing in the node stack consumes the item (clients
detect upgrades from the
:codestate key), so the fork's deposits are removed and theno-runtime-digest-items invariant is documented on
deposit_logandDIGEST_LOGS_SIZE.Documented as accepted limitations (no code change needed)
b570b391) — The on-chain tree may grow to depth 32 whilethe circuits fix
MAX_DEPTH = 16. Documented as a deliberate "fix it when we get close"trade-off on
MAX_TREE_DEPTHand indocs/zk-trie-architecture.md: timeline toexhaustion is years-to-centuries,
LeafCountmakes it observable far in advance, and thecircuit-update path is spelled out.
60b2d4a1,84ee98f8) — No input-size bound andpanic-on-malformed-input in the genesis builder are not vulnerabilities: genesis
construction is an operator-trusted, chain-setup-time boundary, and panicking is the
intended FRAME error channel for
BuildGenesisConfig::build. Documented ingenesis_config_presets.rs.ab976ca9) — The extension's doc overclaimed thatscheduler-dispatched transfers were covered; extensions never run for hook-context
dispatch. The doc now states the real contract: transaction events are scanned,
hook-context credits record explicitly (reversible-transfer execution, mining rewards),
and governance-enacted calls via the scheduler are a known, accepted gap — only Root can
reach that path, and Root can already forge leaves outright via
set_storage.Reviewed, no change
set_high_security— Rejected the proposed existence check: aguardian account may legitimately be funded after enrollment.
Test plan
pallet-wormhole(75),pallet-reversible-transfers,pallet-utility(38),pallet-recovery(35),pallet-multisig(57),frame-system(76) suites pass.quantus-runtimelib (45) + integration (28) suites pass.quantus-nodezk-tree RPC resolver tests pass (7).weight constants hold with large margin.